HADOOP-19212. Avoid Subject.getSubject method on newer JVMs#7081
HADOOP-19212. Avoid Subject.getSubject method on newer JVMs#7081jbrinegar wants to merge 4 commits intoapache:trunkfrom
Conversation
In Java 23, Subject.getSubject requires setting the system property java.security.manager to allow, else it will throw an exception. More detail is available in the release notes: https://jdk.java.net/23/release-notes This is in support of the eventual removal of the security manager, at which point, Subject.getSubject will be removed. Since this project supports older Java releases, and the API which one must migrate to was only introduced in Java 18, I hid the implementations behind a runtime version check. I verified against several local Java versions that the correct classes are loaded (and more importantly, that the incorrect classes are _not_ loaded). The outright removal of the security manager, and by extension the Subject.getSubject method would be a particularly large problem if we do not address this, which was my motivation here. Some manual testing on Java 11, 17, and 23: ``` » java --version openjdk 11.0.23 2024-04-16 LTS » java -cp hadoop-auth-3.5.0-SNAPSHOT.jar org.apache.hadoop.util.subject.SubjectAdapter Current subject is null » java --version openjdk 17.0.5 2022-10-18 LTS » java -cp hadoop-auth-3.5.0-SNAPSHOT.jar org.apache.hadoop.util.subject.SubjectAdapter Current subject is null » java --version openjdk 23 2024-09-17 » java -cp hadoop-auth-3.5.0-SNAPSHOT.jar org.apache.hadoop.util.subject.SubjectAdapter Current subject is null ``` As desired, the class containing the old implementation does not get loaded on a new JVM: ``` » java --version openjdk 23 2024-09-17 » java -verbose:class -cp hadoop-auth-3.5.0-SNAPSHOT.jar org.apache.hadoop.util.subject.SubjectAdapter | \grep -i subject | grep "class,load" | awk '{print $2}' org.apache.hadoop.util.subject.SubjectAdapter org.apache.hadoop.util.subject.HiddenGetSubject javax.security.auth.Subject org.apache.hadoop.util.subject.GetSubjectNg ``` And the inverse is true for an older JVM: ``` » java --version openjdk 17.0.5 2022-10-18 LTS » java -verbose:class -cp hadoop-auth-3.5.0-SNAPSHOT.jar org.apache.hadoop.util.subject.SubjectAdapter | \grep -i subject | grep "class,load" | awk '{print $2}' org.apache.hadoop.util.subject.SubjectAdapter org.apache.hadoop.util.subject.HiddenGetSubject javax.security.auth.Subject org.apache.hadoop.util.subject.ClassicGetSubject javax.security.auth.Subject$1 ```
|
🎊 +1 overall
This message was automatically generated. |
jojochuang
left a comment
jojochuang
left a comment
There was a problem hiding this comment.
Make sense to me. There's a checkstyle warning but glad it's not a big change.
Are you finding other issues with JDK23? I know it's just come out this month.
|
I am happy to report no other issues. This Subject usage in hadoop-common impacts a great many projects, so |
|
Ok sounds like we should get it backported to 3.4.x and 3.3.x too. |
jbrinegar
changed the title
|
@steveloughran PR for HADOOP-19212. I updated the title to reflect the duplicate change. |
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
this is great! we'd been warned about this but nobody had written the code.
I've made some comments. Can you also change the title to that of the previous JIRA on this. Thanks
|
|
||
| GetSubjectNg() { | ||
| try { | ||
| currentMethod = Subject.class.getMethod("current"); |
There was a problem hiding this comment.
This class, and all classes added in this PR, are in the hadoop-auth module because Subject.getSubject is used in both hadoop-auth (KerberosAuthenticator) and hadoop-common (UserGroupInformation). hadoop-common depends on hadoop-auth, so that seemed like a reasonable fit.
DynMethods, however, resides in hadoop-common, and thus it cannot be used in hadoop-auth.
How would you like me to proceed?
There was a problem hiding this comment.
@steveloughran ,
@jbrinegar 's point is valid.
How do you propose solving the circular dependency issue ?
Should the DynMethods code be copied to hadoop-auth ?
Is there some suiteable small external dependency that provides DynMethods ?
There was a problem hiding this comment.
moving Dyn* to hadoop-auth module? then they can be consumed by every hadoop module.
There was a problem hiding this comment.
I agree.
I've just confirmed that hadoop-common already depends on hadoop-auth, so the move should be transparent to all users.
| static { | ||
| int version = 0; | ||
| try { | ||
| version = Integer.parseInt(System.getProperty("java.specification.version")); |
There was a problem hiding this comment.
Use Shell.isJavaVersionAtLeast(18)
There was a problem hiding this comment.
My comment about dependencies applies to Shell as well. Let me know how you'd like to proceed with this one as well.
There was a problem hiding this comment.
IMO testing trying to load the new classes and failing over to the old classes if they cannot be found would be more robust.
There was a problem hiding this comment.
IMO testing trying to load the new classes and failing over to the old classes if they cannot be found would be more robust.
interesting thought.
how about
- we avoid Subject
- pull the string on L33 to a constant
There was a problem hiding this comment.
@stoty interesting thought about attempting and fallbacks
We'd try the java18+ first & fall back to java <= 17 otherwise? or do it in the opposite direction, at least for now?
...-common-project/hadoop-auth/src/main/java/org/apache/hadoop/util/subject/SubjectAdapter.java
Outdated
Show resolved
Hide resolved
...-common-project/hadoop-auth/src/main/java/org/apache/hadoop/util/subject/SubjectAdapter.java
Outdated
Show resolved
Hide resolved
...op-common-project/hadoop-auth/src/main/java/org/apache/hadoop/util/subject/GetSubjectNg.java
Outdated
Show resolved
Hide resolved
|
@steveloughran thank you for the review. I have pushed a second commit that addresses feedback marked with 👍. The title of this PR was updated to include "HADOOP-19212." as a prefix. I am happy to rebase/squash once review is complete, if squashing is necessary. Due to dependencies, I cannot use |
...ommon-project/hadoop-auth/src/main/java/org/apache/hadoop/util/subject/HiddenGetSubject.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
...-common-project/hadoop-auth/src/main/java/org/apache/hadoop/util/subject/SubjectAdapter.java
Outdated
Show resolved
Hide resolved
...mon-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java
Show resolved
Hide resolved
pan3793
left a comment
pan3793
left a comment
There was a problem hiding this comment.
lgtm except for 2 minor issues.
|
JEP 486: Permanently Disable the Security Manager is on the JDK's technical roadmap. As part of the changes (PR in review) will be for Subject.current to throw unconditionally, meaning the workaround to allow a SecurityManager will no longer work. So the timing of the PR here is good. |
|
💔 -1 overall
This message was automatically generated. |
|
I am not sure I have the permissions to retrigger the failing checks. Ultimate failure was on the remote side: This java code compiles just fine: |
| // how getSubject operates depends on the JVM calling it. | ||
| // asserting that it does not throw is a valid test, especially on Java 18 and above | ||
| // prior to Java 18, this method is just a simple wrapper | ||
| assertDoesNotThrow(() -> SubjectAdapter.getSubject()); |
There was a problem hiding this comment.
Use our LambdaTestUtils.intercept() here as it fails better, can validates the type and message
There was a problem hiding this comment.
The comment in #7081 (comment) applies here as well.
| /** | ||
| * Indirectly calls Subject.current(), which exists in Java 18 and above only | ||
| */ | ||
| class SubjectAdapterJava18AndAbove implements HiddenSubjectAdapter { |
There was a problem hiding this comment.
Please use our new DynMethods classes, (which we copied from parquet and which is also found in iceberg). It fails better and as it is so common it's good to get familiar with it
There was a problem hiding this comment.
@steveloughran my comment from here still applies. #7081 (comment)
|
|
||
| import javax.security.auth.Subject; | ||
|
|
||
| public interface HiddenSubjectAdapter { |
There was a problem hiding this comment.
I don't really like the name, maybe rename to JDKSpecificSubjectAdapter or similar ?
cnauroth
left a comment
cnauroth
left a comment
There was a problem hiding this comment.
Thanks for this important patch, @jbrinegar ! I entered a few minor comments.
This is very nitpicky, but there is a comment in test code that still mentions AccessControlContext:
Can you remove mention of that in the comment?
This is a great step, but there will still be references to the deprecated APIs in other parts of the Hadoop codebase. (See below.) Are you interested in addressing all of these, or do you prefer to focus on getting in a hadoop-common patch and then we can focus on other spots piecemeal?
> grep -r --include '*.java' 'AccessController' *
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/client/KerberosAuthenticator.java:import java.security.AccessController;
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/client/KerberosAuthenticator.java: AccessControlContext context = AccessController.getContext();
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/util/PlatformName.java:import java.security.AccessController;
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/util/PlatformName.java: return AccessController.doPrivileged((PrivilegedAction<Boolean>) () -> {
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsConfig.java:import static java.security.AccessController.*;
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/FastByteComparisons.java:import java.security.AccessController;
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/FastByteComparisons.java: theUnsafe = (Unsafe) AccessController.doPrivileged(
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java:import java.security.AccessController;
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java: AccessControlContext context = AccessController.getContext();
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/dynamic/DynMethods.java:import java.security.AccessController;
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/dynamic/DynMethods.java: AccessController.doPrivileged(new MakeAccessible(hidden));
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/dynamic/DynConstructors.java:import java.security.AccessController;
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/dynamic/DynConstructors.java: AccessController.doPrivileged(new MakeAccessible(hidden));
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/CleanerUtil.java:import java.security.AccessController;
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/CleanerUtil.java: final Object hack = AccessController.doPrivileged(
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/CleanerUtil.java: final Throwable error = AccessController.doPrivileged(
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/checker/AbstractFuture.java:import java.security.AccessController;
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/checker/AbstractFuture.java: AccessController.doPrivileged(
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapred/LocalDistributedCacheManager.java:import java.security.AccessController;
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapred/LocalDistributedCacheManager.java: return AccessController.doPrivileged(new PrivilegedAction<ClassLoader>() {
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapred/LocalDistributedCacheManager.java: AccessController.doPrivileged(new PrivilegedAction<Void>() {
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapreduce/v2/util/MRApps.java:import java.security.AccessController;
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-common/src/main/java/org/apache/hadoop/mapreduce/v2/util/MRApps.java: return AccessController.doPrivileged(
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/pipes/Submitter.java:import java.security.AccessController;
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/pipes/Submitter.java: AccessController.doPrivileged(
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timeline-pluginstorage/src/main/java/org/apache/hadoop/yarn/server/timeline/EntityGroupFSTimelineStore.java:import java.security.AccessController;
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timeline-pluginstorage/src/main/java/org/apache/hadoop/yarn/server/timeline/EntityGroupFSTimelineStore.java: return AccessController.doPrivileged(
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/AuxiliaryServiceWithCustomClassLoader.java:import java.security.AccessController;
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/AuxiliaryServiceWithCustomClassLoader.java: return AccessController.doPrivileged(
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.hadoop.util.subject; |
There was a problem hiding this comment.
Can you please add a package-info.java in this new sub-directory and annotate it as private and unstable?
|
|
||
| import javax.security.auth.Subject; | ||
|
|
||
| public interface HiddenSubjectAdapter { |
There was a problem hiding this comment.
Can this be package-private instead of public?
|
Thank you for working on this patch @jbrinegar! This will help eliminate a big blocker for us in trying to upgrade our stack to JDK 23. |
|
@ryantomlinson95 @cnauroth @steveloughran @jbrinegar @jojochuang @pan3793 I'd like to bring to your attention HADOOP-19486, and the corresponding thread I have opened on dev@hadoop.apache.org , where I track my full JDK23 support work. Please check my email, and join the discussion. |
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
commented...I'd added the comments a while back but forgotten to submit the review. sorry
| static { | ||
| int version = 0; | ||
| try { | ||
| version = Integer.parseInt(System.getProperty("java.specification.version")); |
There was a problem hiding this comment.
IMO testing trying to load the new classes and failing over to the old classes if they cannot be found would be more robust.
interesting thought.
how about
- we avoid Subject
- pull the string on L33 to a constant
| static { | ||
| int version = 0; | ||
| try { | ||
| version = Integer.parseInt(System.getProperty("java.specification.version")); |
There was a problem hiding this comment.
@stoty interesting thought about attempting and fallbacks
We'd try the java18+ first & fall back to java <= 17 otherwise? or do it in the opposite direction, at least for now?
|
My alternate patch tries the new API first, and falls back to the old one, @steveloughran . Conversion either way has an overhead, as the Callable / PrivilegedExceptionAction conversion and exception re-wrapping has to happen, but IMO anything we do inside those doAs() blocks is heavyweight enough that the overhad is relatively negligible. |
|
We're closing this stale PR because it has been open for 100 days with no activity. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
9 participants
In Java 23, Subject.getSubject requires setting the system property java.security.manager to allow, else it will throw an exception. More detail is available in the release notes: https://jdk.java.net/23/release-notes
This is in support of the eventual removal of the security manager, at which point, Subject.getSubject will be removed. Since this project supports older Java releases, and the API which one must migrate to was only introduced in Java 18, I hid the implementations behind a runtime version check. I verified against several local Java versions that the correct classes are loaded (and more importantly, that the incorrect classes are not loaded). The outright removal of the security manager, and by extension the Subject.getSubject method would be a particularly large problem if we do not address this, which was my motivation here.
Some manual testing on Java 11, 17, and 23:
As desired, the class containing the old implementation does not get loaded on a new JVM:
And the inverse is true for an older JVM:
Description of PR
How was this patch tested?
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?